HYPERFLEET-1056 - fix: Allow external files in adapter task configmap#172
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Helm helper template named "taskConfigData" that iterates .Values.adapterTaskConfig.data, validates each key (length and RFC1123-like pattern), and emits each entry as a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 72 lines | +0 |
| Sensitive paths | none | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/configmap-adapter-task.yaml`:
- Around line 19-27: The template silently falls back to emitting the file path
when $.Files.Get returns empty; change the logic in the adapterTaskConfig.files
handling so missing chart-file references fail fast instead of falling back:
inside the string branch (the block that sets {{- $chartFile := $.Files.Get
$value }} and currently checks {{- if $chartFile }} ... {{- else }} ...), remove
the fallback branch and replace it with a Helm fail invocation that includes the
file key ($key) and the referenced path/value ($value); keep the existing
behavior for successful $.Files.Get content, and do not alter handling of
non-string (inline/object) values unless you choose to add an explicit separate
inline-content field in values.yaml instead of overloading
adapterTaskConfig.files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66723aa5-98d2-4042-a3d3-f2ecc7d999a2
📒 Files selected for processing (1)
charts/templates/configmap-adapter-task.yaml
|
/retest |
ma-hill
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/templates/configmap-adapter-task.yaml (1)
1-1: ⚡ Quick winNamespace the helper to avoid global collisions.
Helm named templates share a single global namespace across the chart and any subcharts; a bare
taskConfig.datarisks colliding. Other helpers in this chart use thehyperfleet-adapter.prefix.-{{- define "taskConfig.data" -}} +{{- define "hyperfleet-adapter.taskConfig.data" -}}And update the include on Line 17 accordingly.
As per coding guidelines: "Templates use proper Helm conventions (per helm-chart-conventions.md)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/templates/configmap-adapter-task.yaml` at line 1, Rename the Helm named template from "taskConfig.data" to "hyperfleet-adapter.taskConfig.data" to avoid global collisions and follow the chart's helper naming convention, and update any includes/usages of the helper (the include call referenced on line 17) to use the new fully-qualified name "hyperfleet-adapter.taskConfig.data" so the template is referenced correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/configmap-adapter-task.yaml`:
- Around line 1-6: The helm validation helper
helmValidateAdapterTaskIsConfigured currently only checks
.Values.adapterTaskConfig.yaml and .Values.adapterTaskConfig.files and thus
rejects the valid adapterTaskConfig.data-only configuration; update the
predicate in charts/templates/_helpers.tpl to also test for
.Values.adapterTaskConfig.data and adjust the error message text to mention
adapterTaskConfig.data as an accepted option so the documented option4 path is
allowed and correctly described.
---
Nitpick comments:
In `@charts/templates/configmap-adapter-task.yaml`:
- Line 1: Rename the Helm named template from "taskConfig.data" to
"hyperfleet-adapter.taskConfig.data" to avoid global collisions and follow the
chart's helper naming convention, and update any includes/usages of the helper
(the include call referenced on line 17) to use the new fully-qualified name
"hyperfleet-adapter.taskConfig.data" so the template is referenced correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e220406e-1986-41e5-8bc2-64d30f6693f4
📒 Files selected for processing (2)
charts/templates/configmap-adapter-task.yamlcharts/values.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/_helpers.tpl`:
- Around line 141-147: The template currently only rejects
.Values.adapterTaskConfig.data + .Values.adapterTaskConfig.yaml but still allows
.Values.adapterTaskConfig.data + .Values.adapterTaskConfig.files; update the
helper in charts/templates/_helpers.tpl to enforce mutual exclusivity across all
three flags ($hasData, $hasYaml, $hasFiles) by failing when more than one is set
(e.g., add a condition that triggers fail if and $hasData $hasFiles and/or
implement a single-count check that fails when ($hasData + $hasYaml + $hasFiles)
> 1), keeping existing error wording consistent and referencing
.Values.adapterTaskConfig.data, .Values.adapterTaskConfig.yaml, and
.Values.adapterTaskConfig.files.
In `@charts/templates/configmap-adapter-task.yaml`:
- Around line 7-8: The template's ConfigMap key validation (the regexMatch check
using configMapKey and the subsequent fail) wrongly rejects underscores,
breaking documented adapterTaskConfig.data keys like "my_file"; update the regex
used in the regexMatch check to allow underscores in the allowed character class
(so both the start/end and inner character classes permit '_' alongside a-z0-9
and hyphen/dot) or remove this strict RFC1123 subdomain check and replace it
with a permissive pattern that includes underscores to match the documented
contract for config keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 56cfea61-2648-4058-908c-1e8b9156fb5b
📒 Files selected for processing (3)
charts/templates/_helpers.tplcharts/templates/configmap-adapter-task.yamlcharts/values.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/values.yaml
ma-hill
force-pushed
the
HYPERFLEET-1056
branch
3 times, most recently
from
c29e41d to
2560040
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 1c7fe52
into
openshift-hyperfleet:main
2 participants
Summary
Adds support for external configuration files in the adapter task ConfigMap to enable site-specific overrides without modifying the chart. Previously, adapter task configs could only be provided via inline YAML or chart-bundled files, making it difficult to inject environment-specific configurations at deployment time. This change introduces a new
adapterTaskConfig.externalfield that accepts external content via--set-file, allowing operators to combine chart-bundled templates with site-specific configs while preventing key collisions through validation.Jira Issue
HYPERFLEET-1056
Changes
adapterTaskConfig.externalfield to values.yaml as a sibling tofiles(not nested under it)<name>.yamlkeys in the ConfigMap and can be passed via--set-file adapterTaskConfig.external.myconfig=/path/to/file.yamladapterTaskConfig.yamlis now mutually exclusive with bothfilesandexternal(enforced via validation)adapterTaskConfig.filesandadapterTaskConfig.externalcan be used together for combining chart-bundled and site-specific configshyperfleet-adapter.validateTaskConfigKeyshelper template that validates:external(rendered as<name>.yaml) andfileskeysadapterTaskConfig.filesactually exist in the chart (prevents silent failures from typos)helmValidateAdapterTaskIsConfiguredto include external in validation logicNotes
This change is fully backward compatible. Existing Helm values that reference chart-local files (e.g.,
adapterTaskConfig.files.config: "configs/my-config.yaml") will continue to work exactly as before.[HYPERFLEET-1056]: https://redhat.atlassian.net/browse/HYPERFLEET-1056?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Test Plan
make test-helm--set-file adapterTaskConfig.external.myconfig=<path>rendering